fix: Improve type annotations in SignatureVerifier#1845
fix: Improve type annotations in SignatureVerifier#1845BHSPitMonkey wants to merge 2 commits intoslackapi:mainfrom
SignatureVerifier#1845Conversation
Receive headers as abstract/immutable `collections.abc.Mapping` instead of `Dict` to more accurately declare compatibility with dict-like objects (common for holding headers in HTTP frameworks), and mark timestamp/signature args as `Optional` in `is_valid` to reflect reality and fix the typechecker errors in `is_valid_request`.
|
Thanks for the contribution! Before we can merge this, we need @BHSPitMonkey to sign the Salesforce Inc. Contributor License Agreement. |
❌ 2 Tests Failed:
View the top 1 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
WilliamBergamin
left a comment
WilliamBergamin
left a comment
There was a problem hiding this comment.
Hi @BHSPitMonkey thanks for opening this PR 🙏
Love seeing those # type: ignore go away!
Changes seem solid but aren't compatible with python 3.7 and 3.8, I left a comment on how to potentially address it, once resolved we should be good to merge
| self, | ||
| body: Union[str, bytes], | ||
| headers: Dict[str, str], | ||
| headers: Mapping[str, str], |
There was a problem hiding this comment.
For python 3.7 and 3.8 this Mapping type is not importable from collections.abc causing
slack_sdk/signature/__init__.py:30: in SignatureVerifier
headers: Mapping[str, str],
E TypeError: 'ABCMeta' object is not subscriptableImporting Mapping as follows should fix the issue 🤔
from typing import TYPE_CHECKING, Dict, Optional, Union
if TYPE_CHECKING:
from collections.abc import Mapping
else:
Mapping = DictThere was a problem hiding this comment.
If we go this route, it might be a good idea to also include a comment about how this is for python 3.7 and 3.8 backwards compatibility
There was a problem hiding this comment.
Argh, old Pythons strike again! But I like your proposed solution - Will push it with a comment shortly.
Because `Mapping` is not subscriptable until Python 3.9, the previous change introduced runtime errors for 3.7/3.8. Applies suggested workaround from @WilliamBergamin to continue using Dict at runtime until <3.9 is eventually dropped.
2 participants
Summary
Receive headers as abstract/immutable
collections.abc.Mappinginstead ofDictto more accurately declare compatibility with dict-like objects (common for holding headers in HTTP frameworks), and mark timestamp/signature args asOptionalinis_validto reflect reality and fix the typechecker errors inis_valid_request.Testing
These changes only touch type annotations which have no runtime effects; The only expectation here is that type checks pass.
Category
/docs(Documents)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements
python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.